Repository navigation
fix: recover cached venvs from dangling package symlinks - #4318
Conversation
|
I see the CI is failing in quite a few places, could you please check it? |
|
Thanks for flagging this. I checked Buildkite build #18004 and found a Buildifier formatting failure in the new BUILD target ( |
|
Correction to my update above: build #18005 still flagged the same Buildifier check, so my first attribute reorder was incomplete. I moved |
|
Update: build #18006 no longer reports the Buildifier formatting annotation (the only current annotation is a Debian 11 end-of-life warning). The overall Buildkite check still fails: the visible failed jobs are Ubuntu/Bazel 9 and Debian/Bazel 8 and 9; their logs end with a generic Bazel build failure and do not identify a failing target. Read the Docs is still running. I have not treated the remaining jobs as passing. |
|
Thanks for asking me to check the CI. Buildkite #18006 exposed a Pyrefly error in the new regression test: |
|
Update: Buildkite #18007 passed in 11m 33s, Read the Docs succeeded, and both GitHub Actions checks passed. All four reported checks are green on |
rickeylev
left a comment
There was a problem hiding this comment.
Please change the PR description to abide by project guidelines.
|
Also: don't post a comment for every CI run. When you're confident CI is passing and code is fit for review, response PTAL ("Please take another look") or otherwise indicate you believe the code is ready for review again. |
|
PTAL — I updated the PR description to follow the project template and summarized the completed reproduction and checks. |
|
PTAL — I added the requested comment above the symlink check, explaining that a cached venv can leave this parent as a dangling link to an old EXTRACT_ROOT. This is comment-only; the implementation and regression test are unchanged. The prior Buildkite, Read the Docs, and GitHub Actions checks passed on b88ba42; GitHub has started checks for ad0a8b9. |
|
I wonder if there's a similar issue in the script bootstrap? I don't recall the exact logic there. Overall, it'd be nice if we could more automatically invalidate things somehow instead of having to manually check paths and recreate them. The main idea that comes to mind is doing something similar to what zipapp does: it hashes the zipfile and puts that hash as part of the extracted path. Maybe if we hashed the inputs to the create_venv() function and added that as a path prefix? Something to address another time, i think. |
Summary
A persistent venv can retain a dangling package-directory symlink after the Python runtime changes. When initialization then creates a runtime file link beneath that path,
os.makedirs(..., exist_ok=True)raisesFileExistsErrorinstead of recovering.Remove the parent symlink only when it is confirmed dangling, then create the directory and the current runtime file link. Fixes #4269.
Checks
FileExistsErrorbefore the fix.cached_venv_replaces_dangling_package_symlink_testpassed (1 test), verifying that the stale package link becomes a directory and the child link resolves to the current runfiles file.git diff --checkpassed.